Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added(webviews): Adds protocol based webview loading (CODY-3536) #5354

Closed
wants to merge 9 commits into from

Conversation

jamesmcnamara
Copy link
Contributor

@jamesmcnamara jamesmcnamara commented Aug 28, 2024

Adds protocol based webview loading by calling into the extension client via readUriUTF8. This way the extension can dynamically produce the webview assets without them needing to exist in a shared directory on disk.

However this currently only works for the index.html file. The other assets are relativized here and I can't think of a good way to support relativizing those paths to support them same protocol.

My first attempt looked like adding:

if (this.delegate.assetLoader === 'webviewasset') {
    return localResource.with({ scheme: 'webviewasset' })
}

but then the extension client would need to implement some behavior to capture those requests triggered by something like

<script type="module" src="webviewasset://index.js"></script>

which at least in Eclipse wasn't trivial.

Currently I just leave all relative links in place if the webviewasset loader is specified like:

<script type="module" src="./index.js"></script>

and rely on the extension client to be running some sort of asset server that will capture those requests but that's a sneaky bit of undocumented dependency.

Test plan

Ran with local Eclipse build of this branch after deleting all on-disk webviews and confirmed it was served.

Changelog

Allows dynamic serving of webview assets from non-VSCode clients

@jamesmcnamara jamesmcnamara changed the title added(webviews): Adds protocol based webview loading added(webviews): Adds protocol based webview loading (CODY-3536) Aug 29, 2024
Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First up, I don't know what the motivation for this change is. I could guess but I won't. If I understood that better I could probably help more.

With that said, on the face of it, let's not do this.

What's the benefit? Agent/client already need to know where Agent's index.js is on disk... so what if the webview resources come from the filesystem or not?

We don't want resource URIs like webview:// because (I assume) CSP won't understand they are secure.

It is another divergence from VSCode.

It has the potential to make JetBrains CODY_DIR development a different now, because in one case the resources are on disk and in this other case, they're...?

It could be integrated at the webview layer with less effect on the extension code. The native webview provider can give a prefix string when it sets capabilities, and the extension can use the VSCode API's URL rewriter to rewrite to it, without the branching added here.

jamesmcnamara added a commit that referenced this pull request Sep 13, 2024
Adds support for linux for Cody Eclipse. Replacement for
#5354.

There is a bug in the logic for processing the webview index.html file
that was allowing the eclipse plugin to work accidentally, because the
MacOS and Windows integrated browsers were effectively ignoring the
content security policy.

This patches that logic and allows a setting for injecting scripts and
removing the CSP (required for inline-scripts) and allows skipping
converting all embedded links into file:// paths (the eclipse browser
cannot handle custom protocols).

Also fixes two bugs around Java code generation.

## Test plan
Unit tests added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants